Fix typo in error message for runtime validation #9458
Conversation
Summary of ChangesHello @ytetsuro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a minor but important correction in a user-facing error message. The change ensures that when a runtime is not set, the error message accurately refers to the 'engines' field in Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a typo in an error message, changing 'engine' to 'engines' to match the property name in package.json. The change is accurate and improves the clarity of the error message for users. I've added one suggestion to improve code consistency by using template literals for the entire string constant.
| export const RUNTIME_NOT_SET = | ||
| "`runtime` field is required but was not found in firebase.json or package.json.\n" + | ||
| "To fix this, add the following lines to the `functions` section of your firebase.json:\n" + | ||
| `"runtime": "${supported.latest("nodejs")}" or set the "engine" field in package.json\n`; | ||
| `"runtime": "${supported.latest("nodejs")}" or set the "engines" field in package.json\n`; |
There was a problem hiding this comment.
For consistency and better readability, you could use template literals for the entire multi-line string. This avoids mixing different string quoting styles (double quotes and backticks).1
| export const RUNTIME_NOT_SET = | |
| "`runtime` field is required but was not found in firebase.json or package.json.\n" + | |
| "To fix this, add the following lines to the `functions` section of your firebase.json:\n" + | |
| `"runtime": "${supported.latest("nodejs")}" or set the "engine" field in package.json\n`; | |
| `"runtime": "${supported.latest("nodejs")}" or set the "engines" field in package.json\n`; | |
| export const RUNTIME_NOT_SET = | |
| `\`runtime\` field is required but was not found in firebase.json or package.json.\n` + | |
| `To fix this, add the following lines to the \`functions\` section of your firebase.json:\n` + | |
| `"runtime": "${supported.latest("nodejs")}" or set the "engines" field in package.json\n`; |
Style Guide References
Footnotes
-
The Google TypeScript Style Guide recommends using template strings instead of complex string concatenation for better readability, especially for multi-line strings or strings with interpolated values. ↩
There was a problem hiding this comment.
@gemini-code-assist
The original code isn’t written that way — how should I handle this?
There was a problem hiding this comment.
You're right that the surrounding code might not consistently use template literals for all multi-line strings. My suggestion was primarily aimed at improving the readability and consistency of this specific string by using a single string literal type (template literals) throughout, which is a recommended practice in the Google TypeScript Style Guide for multi-line strings or strings with interpolated values.
While it's a stylistic improvement, it's ultimately your decision whether to apply it, especially if there's an established project convention that differs. However, adopting this pattern for new or modified multi-line strings can help gradually improve the codebase's overall consistency and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a typo in an error message, changing engine to engines to match the package.json specification. The refactoring to a template literal is also a good improvement. I've added one suggestion to further enhance the clarity of the error message for a better user experience.
| `\`runtime\` field is required but was not found in firebase.json or package.json. | ||
| To fix this, add the following lines to the \`functions\` section of your firebase.json: | ||
| "runtime": "${supported.latest("nodejs")}" or set the "engines" field in package.json`; |
There was a problem hiding this comment.
While fixing the typo is a great improvement, the error message could be made even clearer. The current phrasing might be ambiguous to users about how to resolve the issue. Presenting the two available options as a numbered list would improve readability and provide a better user experience.
`\`runtime\` field is required but was not found in firebase.json or package.json.\nTo fix this, please specify a runtime in either of the following ways:\n1. Add \`"runtime": "${supported.latest("nodejs")}"\` to the \`functions\` section of your firebase.json.\n2. Set the \`engines\` field in your package.json (e.g. \`"engines": { "node": "20" }\`).`1 participant
Description
Fixed the error message showing the wrong property name.
Please see: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#engines
Scenarios Tested
Sample Commands